Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial implementation of Imviz #429

Merged
merged 24 commits into from
Apr 15, 2021
Merged

Conversation

astrofrog
Copy link
Collaborator

@astrofrog astrofrog commented Feb 16, 2021

This is an experimental PR for now and a work in progress, which aims to implement a very simple imviz viewer (one with one viewer and one with two).

EDIT 2 April 2021: Note that this now requires the latest stable release of bqplot-image-gl as well as this branch of glue-jupyter: glue-viz/glue-jupyter#213. Also I've now added a notebook in this PR to demonstrate the features.

Main to-dos:

  • Figure out how to automatically link the WCSes on data loading - there is infrastructure in glue to do this so it should be a pretty simple call, I can investigate this.
  • Figure out how to avoid having two configurations, one for one-panel imviz and one for two-panel imviz

(I'll update this list as other things come up)

Ticket tracking

  • Finish syncing.
  • Does @astrofrog agree with this?

Fix #513 , fix #515 , fix #517 , fix #522 , fix #523

Partially address #514 (shows the info but not in the desired way), #518, #519, #520

Close https://jira.stsci.edu/browse/JDAT-1192

@astrofrog astrofrog marked this pull request as draft February 16, 2021 17:23
@pllim
Copy link
Contributor

pllim commented Feb 19, 2021

I'll leave my feedback from your first video demo here, for future reference. It was a great demo. Thanks!

  • Would be nice to have all the commands update the existing window instead of creating new displays, but I understand this is a quick "proof of concept"
  • For the region, does it support "markers" like "x", "+", or "*" on click? Does region support only cover 2D shapes?
  • I think we want cursor info outside of the image... though maybe this will be naturally solved if we decide to go down the astrowidgets API via abstract class route... (Initial implementation of abstract class astropy/astrowidgets#126)

@pllim
Copy link
Contributor

pllim commented Feb 22, 2021

@astrofrog
Copy link
Collaborator Author

I've now tidied things up a lot following improvements in bqplot-image-gl and glue-jupyter and made the various pieces of functionality into tools or baked in to the imviz viewer directly. The example notebook is included here. Note that this now requires the latest stable release of bqplot-image-gl as well as this branch of glue-jupyter: glue-viz/glue-jupyter#213

@pllim:

Would be nice to have all the commands update the existing window instead of creating new displays, but I understand this is a quick "proof of concept"

Yes I guess the difficulty is that if the notebook is long enough one would need to keep scrolling between reading text and the single viewer, so this keeps each example next to the viewers. Also as it is once all the cells are run you can go and run the different examples whereas with a single viewer you can't do that. I think it's just a matter of preference - if you want to reorganize the notebook to have only one viewer, that's ok too.

For the region, does it support "markers" like "x", "+", or "*" on click?

No but it would be easy to do so I think - for now this section was meant more as a 'joke' in the sense that DS9 used to just add lots of green circles wherever you clicked even if you didn't want it. But we could easily keep track of the regions and have the coordinates accessible to the user, and also have custom markers.

Does region support only cover 2D shapes?

For now yes - at least in the imviz viewer. What use cases are you thinking of?

I think we want cursor info outside of the image... though maybe this will be naturally solved if we decide to go down the astrowidgets API via abstract class route... (astropy/astrowidgets#126)

We can tweak things like this if needed - I'm not sure if astrowidgets would solve this now that we are just going to think of it as an API wrapper. Here I think instead we might want to change the figure margins and move the label down a bit so it falls in the bottom margin.

@astrofrog
Copy link
Collaborator Author

I've added a running 'todo' list to the description of this PR.

@astrofrog
Copy link
Collaborator Author

astrofrog commented Apr 8, 2021

New demo video based on this PR: https://www.dropbox.com/s/8zc3iox33s5pw7y/imviz_demo_20200408.mov?dl=0

A couple of quick notes:

  • There is a bug with the panning which I am investigating, but I tried to not make it obvious in the video 😆
  • There is a bug with the opacity - when the opacity slider is at max, only one layer should be visible (I'll investigate but won't have time to update the video, this is just a disclaimer in case anyone asks)
  • I don't actually expect users to have to do the manoeuvre with changing the reference data in the second viewer etc - we can make this easier and automatic, but this needs more discussion for now as to how we would like it to behave.

@pllim pllim added the imviz label Apr 8, 2021
Replace Imviz in old notebook with MyImviz
"import matplotlib.pyplot as plt\n",
"\n",
"imviz = ImViz()\n",
"imviz.load_data('jw01072001001_01101_00001_nrcb1_cal.fits')\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace this with something anyone have access to? Like something in astropy-data or STScI Box URL thingy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, for now just using data from data.astropy.org and later we could switch to JWST simulated (or actual eventually!) data

@pllim pllim added this to the Imviz 1.0 milestone Apr 8, 2021
@pllim
Copy link
Contributor

pllim commented Apr 8, 2021

Re: #429 (comment)

I was told by POs that 1D shape isn't a priority right now, so you can ignore me there and stick to 2D regions.

@pllim
Copy link
Contributor

pllim commented Apr 13, 2021

Also see astrofrog#2

@pllim
Copy link
Contributor

pllim commented Apr 13, 2021

Note to self: This blocks https://jira.stsci.edu/browse/JDAT-1406

@astrofrog astrofrog changed the title EXP: Added imviz proof of concepts Add initial implementation of Imviz Apr 15, 2021
@astrofrog astrofrog marked this pull request as ready for review April 15, 2021 10:51
@astrofrog
Copy link
Collaborator Author

astrofrog commented Apr 15, 2021

I've now updated this PR so that the auto-linking happens when the 'match WCS' button is clicked which I think is reasonable. So I think in the interest of moving forward, if the CI passes this can probably be merged and we can open follow-up issues and PRs for your comments @pllim? (merging now means we can then split up the work for the rest)

@pllim
Copy link
Contributor

pllim commented Apr 15, 2021

@astrofrog , I opened some follow-up issues and assigned to you those you explicitly said you were going to investigate. FYI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants